Skip to content

Periodic logging for additional metrics#1094

Merged
rymnc merged 5 commits into
masterfrom
chore/additional-metrics-logging
Aug 31, 2022
Merged

Periodic logging for additional metrics#1094
rymnc merged 5 commits into
masterfrom
chore/additional-metrics-logging

Conversation

@rymnc
Copy link
Copy Markdown
Contributor

@rymnc rymnc commented Aug 29, 2022

Should resolve #984

  • chore: enable metrics logging by default
  • feat(logging): add additional logging metrics, change period to 30 seconds

We now log the following fields:

  • Total connections initiated (peer count) since last log
  • Total Messages
  • Total SWAP peers
  • Total FILTER peers
  • Total errors since last log
  • Total subscribed topics

Should the docs mention which fields refer to values over the last reporting period vs since the node setup?

@rymnc rymnc self-assigned this Aug 29, 2022
@rymnc rymnc requested review from LNSD and jm-clius August 29, 2022 06:06
@rymnc
Copy link
Copy Markdown
Contributor Author

rymnc commented Aug 29, 2022

Additionally, should we log out the metrics using metrics/chronicles_support, as mentioned here, instead of casting them to float64's ourselves?

LNSD
LNSD approved these changes Aug 29, 2022
View reviewed changes
Copy link
Copy Markdown
Contributor

@LNSD LNSD left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A little nitpicking.

LGTM ✅

Comment thread waku/v2/node/wakunode2_setup_metrics.nim
Comment thread waku/v2/node/wakunode2_setup_metrics.nim Outdated
@LNSD
Copy link
Copy Markdown
Contributor

LNSD commented Aug 29, 2022

Tip: Rebase your branch on top of master instead of merging master into your branch to update it with the latest changes. It makes the PRs cleaner.

@rymnc rymnc force-pushed the chore/additional-metrics-logging branch 3 times, most recently from 61d4c91 to 0288fa3 Compare August 30, 2022 04:40
jm-clius
jm-clius requested changes Aug 30, 2022
View reviewed changes
Copy link
Copy Markdown
Contributor

@jm-clius jm-clius left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good job! Congratulations on your first PR. 🥳 I have added some minor comments on log completion, consistency, etc. If possible, could you add an example log output to this PR to get an idea of how it will look when printed to stdout?

Comment thread waku/v2/node/wakunode2_setup_metrics.nim
Comment thread waku/v2/node/wakunode2_setup_metrics.nim Outdated
Comment thread waku/v2/node/wakunode2_setup_metrics.nim Outdated
Comment thread waku/v2/node/wakunode2_setup_metrics.nim Outdated
@rymnc
Copy link
Copy Markdown
Contributor Author

rymnc commented Aug 30, 2022

Thanks for your comments, I'll get those changes in tomorrow

@rymnc
Copy link
Copy Markdown
Contributor Author

rymnc commented Aug 31, 2022

cc: @jm-clius

  • When running wakunode2 with the default config
    image

  • When running wakunode2 with --ports-shift:1 --dns-discovery:true --dns-discovery-url:enrtree://ANTL4SLG2COUILKAPE7EF2BYNL2SHSHVCHLRD5J7ZJLN5R3PRJD2Y@prod.waku.nodes.status.im --discv5-discovery:true
    image

@rymnc rymnc force-pushed the chore/additional-metrics-logging branch from 0288fa3 to 9dd9f2a Compare August 31, 2022 04:02
@rymnc rymnc requested a review from jm-clius August 31, 2022 07:50
jm-clius
jm-clius approved these changes Aug 31, 2022
View reviewed changes
Copy link
Copy Markdown
Contributor

@jm-clius jm-clius left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks! I think this can be merged :)

@rymnc rymnc merged commit ed53bcd into master Aug 31, 2022
@rymnc rymnc deleted the chore/additional-metrics-logging branch August 31, 2022 09:53
@LNSD LNSD mentioned this pull request Nov 7, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jm-clius jm-clius jm-clius approved these changes

+1 more reviewer

@LNSD LNSD LNSD approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@rymnc rymnc

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Logging: Periodic metric/info dump on running nodes

4 participants

@rymnc @LNSD @jm-clius @oskarth